Add email normalization utility and integrate into login flow#5694
Add email normalization utility and integrate into login flow#5694razzeee wants to merge 4 commits intoflathub-infra:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds email-based account merging functionality to allow users with multiple OAuth provider accounts to have their accounts automatically merged when they use the same normalized email address. The normalization strips + aliases and lowercases the email.
- Introduces
normalize_email()utility function for consistent email normalization - Adds
FlathubUser.by_normalized_email()method to find existing users by normalized email across all OAuth providers - Updates OAuth login flow to merge accounts when normalized emails match
- Adds comprehensive test coverage for email normalization and basic login endpoints
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/app/email_utils.py | New utility module with email normalization function |
| backend/tests/test_email_utils.py | Test coverage for email normalization edge cases |
| backend/app/models.py | Adds database query method to find users by normalized email across account types |
| backend/app/logins.py | Integrates email-based merging into OAuth flow logic |
| backend/tests/test_logins.py | Basic smoke tests for login endpoints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| user = login.user | ||
| if user is not None: | ||
| # Eventually we might do user-merge here? | ||
| if user is not None and (merged_user is None or user.id != merged_user.id): |
There was a problem hiding this comment.
This condition allows login when user is logged in and merged_user exists with the same ID, but doesn't handle the case where the account's existing user_entity differs from merged_user. If an account already exists and is linked to a different user, but the normalized email matches yet another user, this could create inconsistent state. Should check if account.user_entity.id != merged_user.id before allowing the merge.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
razzeee
force-pushed
the
merge-accounts
branch
from
3e90cb6 to
a2245d0
Compare
- Implemented `normalize_email` function to standardize email format by removing aliases and converting to lowercase. - Updated login flow to utilize normalized email for account merging. - Added tests for email normalization functionality. - Created tests for login methods and user info retrieval.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
razzeee
force-pushed
the
merge-accounts
branch
from
a2245d0 to
5e7e4e5
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| if not email: | ||
| return email | ||
| if "@" not in email: | ||
| return email |
There was a problem hiding this comment.
The early return when '@' is not present returns the original email unchanged. This could silently allow malformed emails to pass through. Consider either raising an exception for invalid email formats or documenting this behavior explicitly in the docstring, as it may lead to unexpected account merging issues.
| """ | |
| if not email: | |
| return email | |
| if "@" not in email: | |
| return email | |
| Raises: | |
| ValueError: If the email does not contain an '@' character. | |
| """ | |
| if not email: | |
| return email | |
| if "@" not in email: | |
| raise ValueError(f"Invalid email format: '{email}' (missing '@')") |
| normalized_email = ( | ||
| normalize_email(provider_data.email) if provider_data.email else None | ||
| ) | ||
| merged_user = None | ||
| if normalized_email: | ||
| merged_user = models.FlathubUser.by_normalized_email(db, normalized_email) |
There was a problem hiding this comment.
Email normalization is being used for automatic account merging, which could lead to a security issue. If a user controls user+alias@example.com and another user has user@example.com, this normalization could allow the first user to gain access to the second user's account. Email alias normalization should only be applied within the same email provider/domain and with user consent, not automatically for account merging.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
|
||
|
|
||
| def test_deleteuser_not_logged_in(): | ||
| response = client.get("/auth/deleteuser") |
There was a problem hiding this comment.
The HTTP method used here should be DELETE instead of GET for a delete operation. Using GET for a destructive operation violates REST API best practices and could lead to security issues (e.g., accidental deletion via link prefetching).
| response = client.get("/auth/deleteuser") | |
| response = client.delete("/auth/deleteuser") |
| norm_expr = func.lower( | ||
| func.concat( | ||
| func.substring( | ||
| acc_cls.email, | ||
| 1, | ||
| func.nullif(func.position(literal("+"), acc_cls.email) - 1, -1), | ||
| ) | ||
| if func.position(literal("+"), acc_cls.email) > 0 | ||
| else func.substring( | ||
| acc_cls.email, 1, func.position(literal("@"), acc_cls.email) - 1 | ||
| ), | ||
| literal("@"), | ||
| func.substring( | ||
| acc_cls.email, func.position(literal("@"), acc_cls.email) + 1 | ||
| ), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
This SQL-based email normalization duplicates the logic in normalize_email() from email_utils.py, making the code harder to maintain. If the normalization logic changes, both places need to be updated. Consider using a database-side generated column or creating a normalized_email field that's populated using the Python function to ensure consistency.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
normalize_emailfunction to standardize email format by removing aliases and converting to lowercase.